Show graph snapshots in the connection pane#97
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes graph model discovery/rendering in the Connections tree by switching server-side graph model loading to dedicated graph commands and adding client-side snapshot children under each model.
Changes:
- Server: load graph models via
.show graph_models details | project Name, Model, and load snapshots defensively (non-fatal on unsupported clusters). - Client: make graph models collapsible when snapshots exist; render
GraphSnapshotTreeItemchildren and implementgetParentfor snapshots. - Tests: add coverage that documents the
ObjectReader<T>column-binding quirk motivating the server-side projection.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/ServerTests/Features/GraphModelObjectReaderTests.cs | Adds tests around ObjectReader column-binding behavior relevant to graph model loading. |
| src/Server/Schema/ServerSchemaSource.cs | Reworks graph model + snapshot loading to use graph commands and adds defensive error handling/parsing. |
| src/Client/features/connectionsPanel.ts | Adds snapshot tree items under graph models and makes models collapsible when snapshots exist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [TestMethod] | ||
| public void ObjectReader_MisbindsDatabaseNameColumnToNameField_DocumentsBug() | ||
| { | ||
| var dt = new DataTable(); | ||
| dt.Columns.Add("DatabaseName", typeof(string)); | ||
| dt.Columns.Add("Name", typeof(string)); | ||
| dt.Columns.Add("Model", typeof(string)); | ||
| var row = dt.NewRow(); | ||
| row["DatabaseName"] = "Sandbox"; | ||
| row["Name"] = "CallGraph"; | ||
| row["Model"] = "{\"foo\":\"bar\"}"; | ||
| dt.Rows.Add(row); | ||
|
|
||
| using var reader = dt.CreateDataReader(); | ||
| var rows = new ObjectReader<ServerSchemaSource.LoadGraphModelDetailsResult>(reader).ToList(); | ||
|
|
||
| Assert.AreEqual(1, rows.Count); | ||
| Assert.AreEqual("Sandbox", rows[0].Name, | ||
| "If this assertion ever fails, the ObjectReader bug is gone and the explicit projection in CreateGraphModelInfosAsync may be unnecessary."); | ||
| } |
There was a problem hiding this comment.
ObjectReader_MisbindsDatabaseNameColumnToNameField_DocumentsBug hard-asserts the presence of a third‑party bug. If ObjectReader is fixed upstream, this test will start failing even though product behavior would still be correct (and the projection would just become redundant). Consider converting this to an informational/comment-only test (or a test that validates the projection workaround, without asserting the buggy binding as a requirement).
There was a problem hiding this comment.
Removed in commit 9f3a8e0. Also fixed a related build error: the CreateGraphModelInfosAsync call in the full schema load path hadn't been updated to pass the new entityName parameter.
|
@copilot, build failing Error: /home/runner/work/Kusto-Explorer-VsCode/Kusto-Explorer-VsCode/src/Server/Schema/ServerSchemaSource.cs(116,33): error CS7036: There is no argument given that corresponds to the required parameter 'cancellationToken' of 'ServerSchemaSource.CreateGraphModelInfosAsync(IConnection, string?, CancellationToken)' [/home/runner/work/Kusto-Explorer-VsCode/Kusto-Explorer-VsCode/src/Server/Server.csproj] |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Graph models are NOT included in `.show databases entities` results, so they must be | ||
| // fetched via dedicated graph commands instead of being filtered from the entities list. | ||
| var graphModels = await CreateGraphModelInfosAsync(conn, null, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Call to CreateGraphModelInfosAsync is missing the new entityName argument (method now takes IConnection, string?, CancellationToken). As written this won’t compile; pass null for the full database schema load (or plumb an entity name if intended).
| namespace Tests.Features; | ||
|
|
||
| [TestClass] | ||
| public class GraphModelObjectReaderTests |
There was a problem hiding this comment.
Namespace here (KustoVscodeTests) doesn’t match the convention used by the other tests under src/ServerTests/Features (they use namespace Tests.Features;, e.g. ConnectionManagerTests.cs). Aligning the namespace keeps test discovery/organization consistent.
This is fixed at 9f3a8e0. The failing CI log was for the previous commit ( |
Graph model schema now includes snapshot names, but the connections tree still rendered graph models as leaf nodes. Make graph model tree items collapsible when snapshots exist and render each snapshot as a child node. This leaves graph model discovery and snapshot schema loading to the server-side fix already merged on main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danield137
force-pushed
the
user/danield137/GraphModelsAndSnapshots
branch
from
9f3a8e0 to
fac9ea0
Compare
danield137
changed the title
Problem
The server-side graph model/snapshot schema fix is now on main, but the Connections tree still renders graph models as leaf nodes. As a result, snapshot names are present in DatabaseGraphModelInfo.snapshots but cannot be seen in the UI.
Approach
This PR intentionally leaves graph model discovery and snapshot schema loading to the maintainer fix already merged in #98.
Validation
pm run compile
pm run lint
pm test (448/448 tests pass)